-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: Copying contents of deps/npm to test-npm #1853
Conversation
In Ubuntu, `cp -r deps/npm/ test-npm/` was copying `npm` directory under `test-npm` and the structure became `test-npm/npm`. But the test requires the contents of `deps/npm` to be under `test-npm`. Using `deps/npm/.` fixes it.
LGTM. @mhdawson does this also fix your Ubuntu issue with |
LGTM. The fix is also needed on Fedora. |
Would |
@thefourtheye what Ubuntu version is this? I can't seem to reproduce by running |
@silverwind |
@silverwind Mine are as following
I just tried again and I am able to reproduce the problem. |
@thefourtheye reproduced now too. It does only manifest when the target directory exists. Confirmed now on Ubuntu and Arch. |
I am really wondering why this |
It's not shell expansion at least. Also confirming this works on BSD |
Can't we just omit a slash? $ cp -r deps/npm test-npm/ Edit, output from Linux iojs-build-ubuntu1404-64-1 3.13.0-37-generic: $ mkdir -p deps/npm/foo test-npm
$ cp -r deps/npm test-npm/
$ ls -R test-npm
test-npm:
npm
test-npm/npm:
foo
test-npm/npm/foo: |
@jbergstroem that won't work, I think we want |
@silverwind hm, I thought that was what we wanted since we created $ cp -r deps/npm test-npm Edit: You're right. Looks like the script assumes we're in that folder. Then I'd suggest above -- remove the mkdir and just use cp to create it. |
Yeah, that |
Instead of creating `test-npm` first and then copying the contents of `deps/npm`, we just create `test-npm` from `deps/npm` with the `cp -r`
@silverwind @jbergstroem Updated the PR. Can you guys review this now? |
LGTM |
1 similar comment
LGTM |
This fixes a platform inconsistency between BSD and GNU `cp` where `deps/npm` would be copied into a subdirectory of `test-npm` on Linux, but not on OS X. PR-URL: #1853 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Thanks, landed in 1baba05 |
@thefourtheye Would it be possible for you to sign your commits with your real name? You show up as 'thefourtheye' in Author fields now but I see you have a couple of Reviewed-By tags under your full name. |
@bnoordhuis Oh sorry. I changed it in my local |
They're in the history now so that's that. I speculate that the GH editor uses the name and email that are visible in your profile but I don't know for sure, I never use it. Can I suggest you just make your changes from the command line? Almost every change requires that you run |
@bnoordhuis Sure, next time onwards I ll do it from a local branch and sorry about this time. But before submitting the PR, I locally tested the changes and since the change was very small I directly edited it. |
No problem, it's not a big thing, just a small incongruity I noticed. |
This fixes a platform inconsistency between BSD and GNU `cp` where `deps/npm` would be copied into a subdirectory of `test-npm` on Linux, but not on OS X. PR-URL: nodejs/node#1853 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
In Ubuntu,
cp -r deps/npm/ test-npm/
was copyingnpm
directoryunder
test-npm
and the structure becametest-npm/npm
. But the testrequires the contents of
deps/npm
to be undertest-npm
. Usingdeps/npm/.
fixes it.